Repository navigation
Conversation
habohlin
added
the
Needs Review
Liam310
added
Review in progress
| let numOfa = 0; | ||
|
|
||
| for (let i = 0; i < stringOfCharacters.length; i++) { | ||
| if (stringOfCharacters.charAt(i) === findCharacter) { | ||
| numOfa++; | ||
| } | ||
| } | ||
|
|
||
| return numOfa; |
There was a problem hiding this comment.
Variable name numOfa is a little confusing. What does this variable represent exactly? I think you could find a better, more descriptive name.
| if ((num - 11) % 100 === 0) { | ||
| return num + "th"; | ||
| } | ||
|
|
||
| if ((num - 1) % 10 === 0) { | ||
| return num + "st"; | ||
| } | ||
|
|
||
| if ((num - 12) % 100 === 0) { | ||
| return num + "th"; | ||
| } | ||
|
|
||
| if ((num - 2) % 10 === 0) { | ||
| return num + "nd"; | ||
| } | ||
|
|
||
| if ((num - 13) % 100 === 0) { | ||
| return num + "th"; | ||
| } |
There was a problem hiding this comment.
Several of these if statements have the same body:
return num + "th";How can you take advantage of this to make the function a bit shorter and easier to read?
|
|
||
| // Case 2: Numbers ending with 11. Should append "th" to the number | ||
| test("should append 'th' for numbers ending with 11", () => { | ||
| expect(getOrdinalNumber(11)).toEqual("11th"); | ||
| expect(getOrdinalNumber(111)).toEqual("111th"); | ||
| expect(getOrdinalNumber(4211)).toEqual("4211th"); | ||
| }); |
There was a problem hiding this comment.
Great separation of behaviours for this test suite! 👌
| test("should throw error for all invalid numbers", () => { | ||
| expect(() => { | ||
| getOrdinalNumber(5.7); | ||
| }).toThrow("Error: whole number is needed"); | ||
| expect(() => { | ||
| getOrdinalNumber(); | ||
| }).toThrow("Error: whole number is needed"); | ||
| expect(() => { | ||
| getOrdinalNumber("hey"); | ||
| }).toThrow("Error: whole number is needed"); | ||
| expect(() => { | ||
| getOrdinalNumber(undefined); | ||
| }).toThrow("Error: whole number is needed"); | ||
| expect(() => { | ||
| getOrdinalNumber(null); | ||
| }).toThrow("Error: whole number is needed"); |
There was a problem hiding this comment.
I'll always prefer too many test over not enough, but this is probably too many. Additionally, I think the test description is a little confusing. It says "should throw error for all invalid numbers" - but almost all of these are not numbers.
I think there's probably two different tests here:
- Test for actually invalid numbers
- Test for... what do you think?
| // Then it should return a string that contains the original `str` repeated `count` times. | ||
|
|
||
| test("should repeat the string count times", () => { | ||
| test("should repeat the string count times, count is 3", () => { |
There was a problem hiding this comment.
Similar to your last PR, test descriptions aren't there to tell us exactly what the input value is - we can look in the body of the test to see that. Here, "should repeat the string count times" is a generic description of the behaviour that is being tested by the function, so it is a good description on its own. Whether it's 3, 4, or 5 - the point is that the function can repeat a string multiple times.
If you want multiple assertions for the same behaviour that's fine - they ought to be part of the same test
Liam310
added
Reviewed
|
@habohlin Hey Hanna, good work on this one, just a few small notes from me. Also, I am about to go away on annual leave - I'll keep this tab open so I can return to look at your work when I come back, but if there's an urgent need to have a follow-up review, feel free to ask another member of staff and explain the situation 🙂 |
habohlin
added
Needs Review
Learners, PR Template
Self checklist
Task code
CYF-1060
Changelist
Completed the three exercises in Practice TDD with one commit for each added test and one commit for each refactor.